🔴 Showstopper — Python 2 Syntax Errors (4 occurrences)

These will cause a SyntaxError on module load, making the entire application unable to start.

In Python 3, except ValueError, TypeError: is invalid syntax. The correct form is except (ValueError, TypeError): (with parentheses). The comma syntax was removed in Python 3.0.

Locations

1. src/web/main.py — create_viewer_account

# ❌ Current (SyntaxError)
except ValueError, TypeError:
    raise HTTPException(status_code=400, detail="Invalid chat ID format")

# ✅ Fix
except (ValueError, TypeError):
    raise HTTPException(status_code=400, detail="Invalid chat ID format")

2. src/web/main.py — update_viewer_account

# ❌ Same issue
except ValueError, TypeError:

# ✅ Fix
except (ValueError, TypeError):

3. src/db/adapter.py — get_messages_paginated (~line 1135)

# ❌ This replaces a previously-working bare `except:` with broken syntax
except json.JSONDecodeError, TypeError:
    msg["raw_data"] = {}

# ✅ Fix
except (json.JSONDecodeError, TypeError):
    msg["raw_data"] = {}

4. src/db/adapter.py — get_pinned_messages (~line 1510)

# ❌ Same issue
except json.JSONDecodeError, TypeError:

# ✅ Fix
except (json.JSONDecodeError, TypeError):

⚠️ Items 3 and 4 modify existing working code (replacing bare except: clauses). This means merging this PR would break features that currently work on master.

This is the #1 blocker — nothing else matters until these are fixed because the app won't start.

🔴 Security Issues

CRITICAL: S1 — Static /media/ Mount Bypasses All Authentication

The StaticFiles mount serves all backed-up media (photos, videos, documents, voice notes) without any auth check:

app.mount("/media", StaticFiles(directory=config.media_path), name="media")

FastAPI static file mounts don't participate in dependency injection, so require_auth is never called. Any unauthenticated user can directly access:

GET /media/chat_-1001234567890/photo_123.jpg

This also completely bypasses viewer allowed_chat_ids restrictions — a viewer restricted to Chat A can fetch media from Chat B by guessing the path.

Fix: Replace the blanket StaticFiles mount with an authenticated endpoint that checks per-user permissions before serving files. The thumbnail endpoint pattern (dependencies=[Depends(require_auth)]) shows the right approach.

CRITICAL: S2 — Path Traversal in Thumbnail Endpoint

folder and filename in /media/thumb/{size}/{folder:path}/{filename} are not validated against .. sequences. Starlette's {folder:path} converter accepts directory traversal:

GET /media/thumb/200/../../etc/hostname/something.png

Path(media_root) / "../../etc" resolves outside the media directory. While _is_image() restricts to image extensions, any file with .jpg/.png extension anywhere on the filesystem can be read and thumbnailed. The thumbnail is also written to an attacker-controlled path.

Fix (in thumbnails.py):

source = (media_root / folder / filename).resolve()
if not source.is_relative_to(media_root.resolve()):
    return None

dest = _thumb_path(media_root, size, folder, filename).resolve()
if not dest.is_relative_to(media_root.resolve()):
    return None

HIGH: S3 — Non-Constant-Time Master Token Comparison

if AUTH_TOKEN and auth_cookie == AUTH_TOKEN:

Python's == for strings short-circuits on the first differing byte. Since the master token is permanent and deterministic (derived from credentials with a hardcoded salt), a timing attack could extract it byte-by-byte over the network.

Viewer password verification correctly uses secrets.compare_digest, but the master token check does not.

Fix:

if AUTH_TOKEN and secrets.compare_digest(auth_cookie, AUTH_TOKEN):

HIGH: S4 — Deterministic, Non-Rotating Master Token

The master AUTH_TOKEN is derived deterministically:

AUTH_TOKEN = hashlib.pbkdf2_hmac("sha256", f"{username}:{password}".encode(), b"telegram-archive-viewer", 600_000).hex()

This means:

• Token never changes until env vars change (requires restart)
• Token is identical across all deployments with the same credentials
• /api/logout only clears viewer sessions — master token cannot be invalidated
• Anyone who obtains the token (DevTools, logs, HTTP interception) has permanent access

Suggested fix: Generate a random session token for the master on each login (like viewers get), store it in the session dict. This enables real logout, rotation, and revocation.

HIGH: S5 — No Rate Limiting on Login

No rate limiting, account lockout, or progressive delay on POST /api/login. Combined with the 4-character minimum password, the viewer password space is brute-forceable in minutes.

Suggested fix: Add per-IP rate limiting (e.g., slowapi library) or per-username lockout after N failures.

MEDIUM: S6 — No CSRF Protection

Admin CRUD endpoints rely solely on SameSite=lax cookies. While this blocks form-based CSRF, it doesn't protect against fetch() from an allowed CORS origin with credentials: 'include'. Consider adding a X-CSRF-Token header check.

MEDIUM: S7 — Audit Logging Only for Viewers, Not Master

All admin actions (creating/deleting viewers, changing passwords) by the master user are not audit-logged. A compromised master account operates with zero forensic trail.

🟡 Database & Migration Issues

HIGH: Migration 007 Revision Collision

The CHANGELOG in this PR documents two different features both claiming migration 007:

• v7.0.0: "Alembic Migration 007 — Database schema for transactions table"
• v7.1.0: "Alembic Migration 007 — Database schema for viewer_accounts and viewer_audit_log tables"

Only the viewer_accounts migration file exists in this PR. The transactions migration/code is documented in the CHANGELOG but does not exist anywhere in the PR. If a transactions migration 007 is planned for a separate branch, merging both would cause Alembic to crash with "Multiple head revisions," bricking the application on startup.

Current master has migrations 001–006. This needs to be clarified:

• Does the transactions feature (with its own migration 007) exist elsewhere?
• If not, the v7.0 CHANGELOG entry should be corrected (see Scope comment)

Fix: Ensure no revision collision. If needed, renumber to 008.

MEDIUM: Redundant Index on username

Both the migration and model create an explicit index idx_viewer_accounts_username on username, but the UNIQUE constraint already creates a unique index automatically in both PostgreSQL and SQLite. This doubles write overhead for zero benefit.

# Migration:
sa.UniqueConstraint("username"),  # ← already creates an index
op.create_index("idx_viewer_accounts_username", "viewer_accounts", ["username"])  # ← redundant

# Model:
username: Mapped[str] = mapped_column(String(255), unique=True, ...)  # ← already indexed
__table_args__ = (Index("idx_viewer_accounts_username", "username"),)  # ← redundant

Fix: Remove op.create_index("idx_viewer_accounts_username", ...) from the migration and Index("idx_viewer_accounts_username", "username") from the model.

MEDIUM: Adapter Uses Wrong Session Context Manager

All 50+ existing adapter methods use self.db_manager.async_session_factory() with explicit commits. The 7 new viewer methods use self.db_manager.get_session() instead. get_session() auto-commits on block exit, so the explicit await session.commit() in the new methods is a redundant double-commit.

Not broken, but inconsistent with the rest of the codebase and confusing for future contributors.

Fix: Replace self.db_manager.get_session() with self.db_manager.async_session_factory() in all new viewer adapter methods to match the existing pattern.

MEDIUM: No FK on viewer_audit_log.viewer_id, No Purge Mechanism

I understand the intent — audit records should survive viewer deletion (the denormalized username column confirms this). However:

1. This intent isn't documented anywhere (a future contributor might "fix" it with CASCADE, destroying audit history)
2. The audit log grows unbounded with no purge mechanism

Also, delete_viewer_account in the adapter doesn't log the deletion itself to the audit trail — so the act of removing a viewer is invisible.

LOW: datetime.utcnow() Deprecated

Used in new models and adapter code. I know the existing codebase uses it too — this is consistent. But FYI, it's deprecated since Python 3.12 and will eventually be removed. We can address this project-wide separately.

LOW: create_viewer_account Doesn't Handle IntegrityError

Two concurrent requests creating the same username would bypass the uniqueness check (which runs before insert) and hit the DB constraint, resulting in an unhandled 500 error. The retry_on_locked() decorator only retries on "locked"/"connection" errors, not IntegrityError.

Fix: Catch IntegrityError and return a proper 409 response.

🟡 Logic & Correctness Issues

HIGH: Frontend/Backend Parameter Mismatch — Sender Search Broken

The frontend's search filter UI has a "Sender name..." text input, but sends it as sender_id:

// Frontend (index.html):
if (f.sender) url += `&sender_id=${encodeURIComponent(f.sender)}`
//                      ^^^^^^^^^ sends text value like "John" as sender_id

The backend get_messages endpoint has sender_id: int | None — passing a text name will either return a 422 Unprocessable Entity or be silently ignored.

Meanwhile, the global search endpoint correctly uses sender (string match against User.first_name/last_name/username), which shows the right approach.

Fix: Change frontend to use sender parameter for per-chat message search, or add a string-based sender name filter parameter to the backend.

MEDIUM: In-Memory Sessions — No Bounds, No Proactive Cleanup, Lost on Restart

The _viewer_sessions dict has several issues:

1. No proactive cleanup — Sessions are only evicted on access. Repeated logins create new entries without invalidating old ones, causing unbounded memory growth.
2. Lost on restart — All viewer sessions disappear when the process restarts, forcing all viewers to re-login.
3. Not shared across workers — If uvicorn runs with multiple workers, each has its own dict. A viewer authenticated on worker A is unauthenticated on worker B.

Suggested fix: Add a max sessions per user (evict oldest on new login), add periodic background cleanup, and consider database-backed sessions if persistence matters.

MEDIUM: is_active Not Checked on Session Validation

_get_current_user checks the in-memory session but doesn't re-validate is_active from the database. A deactivated viewer's existing sessions remain valid for up to 24 hours.

I see that update_viewer_account invalidates sessions on changes — that's good. But if is_active is set to 0 via a direct DB update (or if the invalidation has a race condition), the deactivated user retains access.

MEDIUM: Session/Cookie TTL Mismatch

Server-side sessions expire in 24h (_SESSION_MAX_AGE = 86400) but cookies can live up to 30 days (AUTH_SESSION_SECONDS from AUTH_SESSION_DAYS). After 24h the client holds a cookie that always returns 401, with no user-facing explanation.

LOW: LIKE Wildcards Not Escaped in Search

stmt = stmt.where(Message.text.ilike(f"%{query}%"))

Characters % and _ in user input aren't escaped. Searching for % matches all messages, _ matches any single character. Not a security issue (SQLAlchemy parameterizes), but a UX bug.

Fix:

escaped = query.replace("%", r"\%").replace("_", r"\_")
stmt = stmt.where(Message.text.ilike(f"%{escaped}%", escape="\\"))

LOW: Error Details Leaked in 500 Responses

Multiple endpoints return detail=str(e) which can expose internal exception messages (SQL queries, file paths, schema info) to clients:

raise HTTPException(status_code=500, detail=str(e))

Fix: Return generic "Internal server error" to clients, log the full exception server-side.

🧪 Test Suite Assessment

The PR description claims "28 new tests across 6 classes, 105 total tests pass." After reviewing the test code, I have concerns about the quality and coverage of these tests.

The Core Problem: 0% Functional Coverage

Not a single test imports or calls any production auth function. The tests validate Python's hashlib, json, secrets, and dict — not the application's authentication system.

Here's the breakdown:

What the Tests Actually Do

Most tests construct a Python dict and assert that the keys they just set exist:

def test_auth_check_response_structure(self):
    response_disabled = {"authenticated": True, "auth_required": False}
    assert "authenticated" in response_disabled   # ← always true, tests nothing

Or they test Python stdlib functions:

def test_hash_password_produces_hex(self):
    hash_bytes = hashlib.pbkdf2_hmac("sha256", b"testpass", ...)  # ← tests hashlib, not the app
    assert len(password_hash) == 64   # ← SHA256 always produces 64 hex chars

Or they re-implement the production logic in test code instead of importing it:

def test_master_no_display_ids_sees_all(self):
    user = {"role": "master", "allowed_chat_ids": None}
    if user["role"] == "master":
        result = display_chat_ids if display_chat_ids else None
    assert result is None   # ← tests the test's own logic, not _get_user_chat_ids()

These tests would pass identically whether the auth code is correct, broken, or deleted entirely.

What Tests Should Exist

At minimum, the following are needed before this can be considered tested:

Integration tests (FastAPI TestClient):

• POST /api/login with valid creds → 200, sets cookie
• POST /api/login with invalid creds → 401, no cookie
• GET /api/chats without cookie → 401 (when auth enabled)
• GET /api/chats with valid cookie → 200
• Auth disabled → all endpoints accessible without cookies
• Viewer can only access allowed chats
• Admin endpoints return 403 for viewer role

Unit tests importing actual production code:

• _hash_password() and _verify_password() roundtrip
• _get_current_user() with valid/invalid/expired sessions
• require_auth() dependency behavior
• _get_user_chat_ids() for master vs viewer vs auth-disabled

Security tests:

• Cookie has httponly=True and samesite=lax
• Expired sessions are rejected
• WebSocket enforces auth (currently doesn't — real bug)

📦 Scope & Housekeeping

The PR Bundles 3+ Unrelated Features

The branch name feat/web-viewer-enhancements and the commit history reveal this is a multi-feature development branch, not a single-feature PR:

Feature A — UI Enhancements ("v7.0"): Bubble tails, media gallery, global search, keyboard shortcuts, skeleton loading, thumbnails, XSS fix, hash routing, lightbox, message deep links.

Feature B — Multi-User Auth ("v7.1"): Viewer accounts, admin UI, PBKDF2 hashing, per-user chat filtering, audit logging.

Feature C — Documentation/Planning (~4,000 lines): 20 files under plans/ (phase docs, research notes, AI code review reports), 6 docs under docs/.

These should be separate PRs for:

• Easier review (9,391 lines is really hard to review as one unit)
• Safe rollback (if auth has a bug, we don't want to rollback the UI improvements too)
• Clearer git history

plans/ Directory Should Not Be Committed

The 20 files under plans/ are development artifacts (phase plans, research notes, AI-generated code review reports). These don't belong in the repository — they're working documents, not project documentation.

Fix: Remove the plans/ directory from the PR.

CHANGELOG Describes Phantom v7.0 Features

The v7.0 CHANGELOG entry describes features that do not exist in this PR:

• "Transaction Accounting View — Auto-detect monetary transactions from chat messages"
• "Transaction Detection Module (src/transaction_detector.py)"
• "Alembic Migration 007 — Database schema for transactions table"
• "30+ New API Endpoints" (including /api/chats/{chat_id}/transactions/*)

None of this code is in the PR. The CHANGELOG should only document what's actually being shipped. This is confusing for users reading the changelog to understand what changed.

Fix: Remove the transaction-related entries from the v7.0 CHANGELOG section. If these are planned for a future PR, they can be added then.

Footer Credit

I see the latest commit (co-authored) addresses the footer credit. Just want to confirm the current state is:

Made by GeiserX · Contributed by phenix

That works for me — just wanted to call it out since the intermediate state removed the sponsor link.

Dockerfile.viewer: Pillow Dependency

Adding libjpeg62-turbo-dev and libwebp-dev for thumbnail support increases the viewer image size. The Pillow>=10.0.0 dependency is also unpinned on the upper bound — a Pillow 12.x breaking change could silently break builds.

Suggested fix: Pin to a range like Pillow>=10.0.0,<12.0.0.

Summary of Required Changes

Here's a checklist of what needs to be addressed, roughly in priority order:

• Fix Python 2 except syntax in 4 places (showstopper)
• Add path traversal protection to thumbnail endpoint (critical security)
• Add auth to static media mount or replace with authenticated endpoint (critical security)
• Use secrets.compare_digest for master token comparison (security)
• Fix sender_id vs sender frontend/backend mismatch (broken feature)
• Clarify/fix migration 007 revision numbering
• Write real integration tests with FastAPI TestClient
• Remove plans/ directory from PR
• Clean up CHANGELOG (remove phantom transaction entries)
• Remove redundant index on viewer_accounts.username
• Use async_session_factory() instead of get_session() in adapter
• Consider splitting into separate PRs (UI enhancements vs auth)

Optional but recommended:

• Add rate limiting to login endpoint
• Add session bounds / proactive cleanup
• Increase minimum password length to 8
• Add CSRF token mechanism
• Generate random master session tokens (enable real logout)
• Escape LIKE wildcards in search
• Use generic error messages in 500 responses

Happy to discuss any of these. The feature is solid in concept — just needs this rework to be production-ready. Thanks again for the contribution! 🙌

Hey @PhenixStar — thanks for putting this together and for the discussion. Multi-user viewer access control has been on the roadmap, and your PR helped crystallize the requirements.

After the thorough review, the number of issues (syntax errors, security gaps, test coverage, bundled features) makes it more practical to implement this from scratch rather than iterate on the current branch. I'm closing this PR and will ship the feature as part of v7.0.0, addressing all the points raised in the review.

Thanks again for the initiative — I'll credit you in the release notes for the idea and discussion.